fix(ttx): revert IsMe gate — breaks after vault-wiping restart#1564
fix(ttx): revert IsMe gate — breaks after vault-wiping restart#1564atharrva01 wants to merge 6 commits into
Conversation
|
hi @adecaro , I gated |
59e2fcc to
f5bf435
Compare
|
Hi @atharrva01 , thank you for submitting this and sorry for the late reply 🙏 I'll review ASAP. I'm curious to look at your approach more carefully 😄 |
f5bf435 to
6417f23
Compare
|
Hi @atharrva01 , apologies for this late reply. I just want to double check the semantic. |
|
Hi @adecaro! You're right to flag this. The issue is that I fixed it by calling Also, for idemixnym identities specifically, All identity and ttx tests pass. |
|
Hi @atharrva01 , I think we are still not there. We need to check the function func (p *KeyManager) DeserializeSigningIdentity(ctx context.Context, raw []byte) (tdriver.SigningIdentity, error) {
id, err := p.Deserialize(ctx, raw)
if err != nil {
return nil, err
}p.Deserialize, the code verifies the identity itself against the issuer public key. So, I would start with a unit-test that checks that What do you think? |
…edger-labs#1226) Gate GetSigner() behind a cheap IsMe() check to skip the expensive idemix sign-and-verify deserialization for remote identities. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
… testifylint Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…e is correct after restart Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…Identity p.Deserialize already verifies the ZK association proof against the issuer public key, which is sufficient to reject identities from a different issuer. The ephemeral sign-and-verify liveness check is no longer needed. Added TestDeserialize_RejectsDifferentIssuerIdentity to prove p.Deserialize fails for different-issuer identities before any signing identity is built. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
f4c758d to
53b0579
Compare
|
@adecaro, implemented the suggested approach. Added a unit test (TestDeserialize_RejectsDifferentIssuerIdentity) confirming that p.Deserialize already rejects identities from a different issuer via verifyProof(), returning cannot deserialize, invalid identity. One thing I noticed: identities from the same issuer but belonging to another user now deserialize successfully, since ownership of the nym key is no longer checked there. In a real deployment, Sign would still fail because the local CSP does not have the corresponding nym secret key. That’s why I kept the earlier IsMe guard, without it, same-issuer remote signers end up causing transaction failures instead of falling back to remote signing. Happy to adjust if you’d prefer a different direction. |
After a node restart the in-memory signers cache is empty, so IsMe() falls back to GetExistingSignerInfo in storage. RegisterIdentityDescriptor was only persisting the raw identity via StoreSignerInfo, but tokens store the typed (alias) identity as their owner. On restart IsMe(typedIdent) returned false, the IsMe gate in collectendorsements skipped local signing, and the transfer ended with a chaincode-rejected ZK proof. Fix: also call StoreSignerInfo for the alias when it is set, mirroring what updateCaches already does in memory for both raw and alias keys. This is the root cause of all dlog CI failures on this branch: every failing test suite restarts at least one node before the transfer under test, while the passing suites do not. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
…removal DeserializeSigningIdentity no longer runs a sign-and-verify round-trip (removed in b43ba2b), so the IsMe gate that was added to avoid that expense is no longer necessary. More critically, the gate breaks after a vault-wiping restart: the DB is wiped, IsMe() returns false for all identities (including the node's own token owners), local signing is skipped, and the idemix ZK proof fails with "contribution is not zero" because the wrong code path is taken. Revert to unconditional GetSigner(): for remote identities it fails fast with a CSP key-not-found, same as before, and for local identities it correctly signs regardless of whether the DB has been wiped. Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
Closes #1226
Originally this PR gated
GetSigner()behindIsMe()to avoid an expensive sign-and-verify round trip insideDeserializeSigningIdentityfor remote idemix identities.That optimization turned out to break zkatdlog integration tests: when a node restarts with vault deletion, the DB is wiped,
IsMe()returns false for every identity (including the node's own token owners), local signing gets skipped, and the idemix ZK proof fails withcontribution is not zero.Meanwhile, the expensive sign-and-verify inside
DeserializeSigningIdentitywas independently removed in b43ba2b , so the motivation for the gate is gone too.Fix: revert to unconditional
GetSigner(). For remote identities it fails fast with a CSP key-not-found; for local ones it signs correctly regardless of DB state. Also persists signer info for the typed alias inRegisterIdentityDescriptorsoIsMe()stays correct across restarts even when used elsewhere.